/writing-tests skill + oxlint rule + test cleanup#45
Merged
kostyafarber merged 4 commits intomainfrom Apr 26, 2026
Merged
Conversation
The /writing-tests skill is the source of truth for how to write tests in this codebase — how to pick a category, templates for each, the fake-test checklist, and the banned patterns (counting invocations, hand-built events, global stubs, parallel-world test harnesses). It also tells agents to research prior art online (VSCode, Obsidian, Signal, Bitwarden) before inventing a mock. Claude.md Testing section shrinks to a single sentence pointing at the skill, since the full guidance belongs in one place that's triggered when tests are being written — not duplicated in the root constitution.
Ban vi.stubGlobal in .test.ts files via a new shift-plugin rule. Global stubbing is order-sensitive, leaks across tests if unstub is missed, and hides production code's unmanaged global dependencies — the skill's "inject the boundary" rule catches it at review, this rule catches it at lint. The one offender, EdgePanManager.test.ts, was migrated to drive through TestEditor. It previously stubbed requestAnimationFrame and asserted on handlePointerMove spy args. The rewrite starts a real hand-tool drag, triggers updateEdgePan, and asserts on the observable viewport pan change. Two tests (pans during drag, no-op when not dragging). No rAF stub, no spy, no vi.fn.
File tested `Selection` from `@/types/selection` but lived in `managers/` under a name implying a `SelectionManager` class that doesn't exist. Moves next to the source, matches the class it actually covers. Pure rename — no content changes. Per testing-strategy.md scope item #4.
Cleans up the five remaining legacy offenders of the /writing-tests skill's banned patterns. All now use closure capture (payload arrays, flags) or closure counters instead of vi.fn / vi.spyOn / toHaveBeenCalled. - lifecycle.test.ts — EventEmitter tests capture payloads in arrays. - signal.test.ts — reactive library tests use closure counters (`let fires = 0; ...; fires++`). Count IS the library's contract. - BaseTool.test.ts (renamed from BaseTool.contract.test.ts) — lifecycle contract tests capture every (prev, next, event) triple in an array. - layout.test.ts — the three bbox-optimization assertions use closure counters. The createMockFont parallel-world hack is flagged via comment and tracked in projects/shift/text-layout-rethink.md. - KeyboardRouter.test.ts — full rewrite through TestEditor. Drives through a real editor and asserts on observable state: editor.zoom, editor.getActiveTool(), editor.pointCount, editor.clipboardBuffer, editor.toolManager.activeToolId. Broader oxlint rule shift/no-vitest-mock-primitives fires on vi.fn, vi.spyOn, toHaveBeenCalled*, and .mock.calls in .test.ts files. Zero exemptions — all migrations landed. Skill updated: the banned-patterns row now explicitly lists vitest primitives and carves out closure counters for primitives whose contract IS the invocation count.
kostyafarber
force-pushed
the
firmclaw/writing-tests-skill
branch
from
997a922 to
410c8a5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the testing conventions this repo has accumulated into a first-class, discoverable, and enforced thing.
Three commits
Add
/writing-testsskill; collapseClaude.mdTesting to a pointer.claude/skills/writing-tests/SKILL.mdis now the source of truth: the rule ("assert on observable state, not mock calls"), when to stop and rethink (4-row table of banned patterns), the 4 test categories (Tool / Command / Pure module / Bridge), templates, the fake-test checklist, and why these rules exist (links to the original sweep commits).SystemClipboardadapter in this repo came from exactly that pattern.Claude.mdTesting section collapses from 20 lines to a single sentence pointing at the skill.Add oxlint
shift/no-vi-stub-global-in-tests; migrateEdgePanManagertestvi.stubGlobalin any.test.tsis an error. Catches the "I'll just monkey-patchwindow.electronAPI/requestAnimationFrame" reflex at CI, not at review.EdgePanManager.test.tswas the one offender. Rewrote it throughTestEditor: start a real hand-tool drag, triggereditor.updateEdgePan(...), assert on the observable viewport pan change. No rAF stub, no spy, novi.fn. Two tests (pans during drag, no-op when not dragging).Rename
SelectionManager.test.ts→types/selection.test.tsSelectionfrom@/types/selectionbut lived inmanagers/under the wrong name. Pure rename, no content changes. Per testing-strategy.md Ruler tool #4.Out of scope (follow-up)
The skill's banned-pattern list covers more than just
vi.stubGlobal. Five other test files still usevi.spyOn/toHaveBeenCalled/ counter variables and would need migration or rewrite:BaseTool.contract.test.ts— arguably legitimate (lifecycle contract)signal.test.ts— legitimate (reactive library fire counts ARE the contract)lifecycle.test.ts— EventEmitter; closure-capture rewrite is easylayout.test.ts—pathHitTesterspy; needs migration to observable outputKeyboardRouter.test.ts— needs inspectionEach has a different disposition (migrate / keep-and-exempt / delete). A follow-up PR should sweep these with a broader lint rule (
no-mock-call-assertions) once the migrations are in.Memory
Also updated
feedback_no_spy_count_tests.mdin personal memory to point at the skill instead of duplicating the content — skill is now the source of truth.Test plan
pnpm test— 539 tests passpnpm lint:check— 0 errors, 0 warnings (106 rules, was 105)pnpm typecheck— cleanvi.stubGlobalin a fresh.test.tsfires the expected error